Skip to content

Allow deletion of system VM templates#8556

Merged
JoaoJandre merged 3 commits intoapache:mainfrom
scclouds:allow_delete_systemVM_templates
Aug 15, 2024
Merged

Allow deletion of system VM templates#8556
JoaoJandre merged 3 commits intoapache:mainfrom
scclouds:allow_delete_systemVM_templates

Conversation

@GaOrtiga
Copy link
Contributor

Description

Currently, ACS does not allow the deletion of system VM templates. However, it might be of interest to the operator to delete templates from older versions that are no longer in use.

A change was applied to remove the restriction on the deletion of system VM templates, allowing operators to delete such templates.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

How Has This Been Tested?

I applied the changes and successfully deleted old system VM templates

@weizhouapache
Copy link
Member

@GaOrtiga
If you update the template to USER, then you will be able to remove it.
this will prevent some human mistakes

@codecov
Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 30.77%. Comparing base (1925040) to head (d30510f).
Report is 594 commits behind head on main.

Files Patch % Lines
.../com/cloud/template/HypervisorTemplateAdapter.java 0.00% 1 Missing and 1 partial ⚠️
...k/api/command/user/template/DeleteTemplateCmd.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8556      +/-   ##
============================================
- Coverage     30.82%   30.77%   -0.05%     
+ Complexity    34014    33967      -47     
============================================
  Files          5341     5341              
  Lines        375027   375034       +7     
  Branches      54554    54554              
============================================
- Hits         115592   115410     -182     
- Misses       244155   244363     +208     
+ Partials      15280    15261      -19     
Flag Coverage Δ
simulator-marvin-tests 24.63% <0.00%> (-0.08%) ⬇️
uitests 4.39% <ø> (ø)
unit-tests 16.49% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@weizhouapache
Copy link
Member

@GaOrtiga If you update the template to USER, then you will be able to remove it. this will prevent some human mistakes

@GaOrtiga @DaanHoogland
do you agree and close this PR ?

@GaOrtiga
Copy link
Contributor Author

@GaOrtiga If you update the template to USER, then you will be able to remove it. this will prevent some human mistakes

@weizhouapache I understand that there is a way to circumvent this restriction, however, since this would only be possible to Root Admins, I believe deleting it directly should still be an option. To address your concerns about this change, I could make the force flag necessary when the template's type is SYSTEM.

Would this measure be enough to offset the concerns regarding this implementation?

Comment on lines -751 to -753
if (template.getTemplateType() == TemplateType.SYSTEM) {
throw new InvalidParameterValueException("The DomR template cannot be deleted.");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        if (template.getTemplateType() == TemplateType.SYSTEM && ! cmd.isForced()) {
            throw new InvalidParameterValueException("The DomR template cannot be deleted.");
        }

is what you mean @GaOrtiga ?

Copy link
Contributor Author

@GaOrtiga GaOrtiga Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and I will also update the message in the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        if (template.getTemplateType() == TemplateType.SYSTEM && ! cmd.isForced()) {
            throw new InvalidParameterValueException("The DomR template cannot be deleted.");
        }

is what you mean @GaOrtiga ?

@DaanHoogland @weizhouapache given that this operation can only be carried out by Root Admins, could we proceed with this proposed implementation?

@weizhouapache
Copy link
Member

@GaOrtiga @DaanHoogland
sorry this PR is not needed in my opinion

@DaanHoogland
Copy link
Contributor

@GaOrtiga as a user wants this convenience. Even when I agree that it is not vital, that would be enough for it to be needed.

  • Is there any more pressing objection @weizhouapache?
  • Alternatively, is there a modus operandus that would make for a more acceptable change?

@weizhouapache
Copy link
Member

@GaOrtiga as a user wants this convenience. Even when I agree that it is not vital, that would be enough for it to be needed.

  • Is there any more pressing objection @weizhouapache?
  • Alternatively, is there a modus operandus that would make for a more acceptable change?

@DaanHoogland
SYSTEM template is very important for cloudstack environments. We need to be very careful to remove it
a force flag is not enough to me.

since we already have a way to delete a SYSTEM template (see #8556 (comment)), this change is unnecessary.

my question to @GaOrtiga @DaanHoogland
is there special use case that we must be able to delete a SYSTEM template in one step ?

@weizhouapache
Copy link
Member

@DaanHoogland @GaOrtiga
let me explain a bit more
users might delete the unused SYSTEM template one or twice every year by average. It takes just few minutes.

However, with this PR, they will be able to delete SYSTEM template by a single API/command.
If the SYSTEM template is removed by mistake, it will be a critical issue .
can you compare the pros and cons ?

@GaOrtiga
Copy link
Contributor Author

@GaOrtiga @DaanHoogland sorry this PR is not needed in my opinion

Having a workaround should not be a reason to restrict an operation. If the operator does not know that you can circumvent this restriction they may feel obliged to perform the operation directly in the DB, especially since the exception thrown is very vague and editing the template's type to make it deletable is very non-intuitive.

@GaOrtiga
Copy link
Contributor Author

@GaOrtiga as a user wants this convenience. Even when I agree that it is not vital, that would be enough for it to be needed.

  • Is there any more pressing objection @weizhouapache?
  • Alternatively, is there a modus operandus that would make for a more acceptable change?

@DaanHoogland SYSTEM template is very important for cloudstack environments. We need to be very careful to remove it a force flag is not enough to me.

since we already have a way to delete a SYSTEM template (see #8556 (comment)), this change is unnecessary.

my question to @GaOrtiga @DaanHoogland is there special use case that we must be able to delete a SYSTEM template in one step ?

The use case would be any operator that wants to delete SYSTEM templates and does not know about the workaround, since it's not really something self-explanatory.

@weizhouapache
Copy link
Member

The use case would be any operator that wants to delete SYSTEM templates and does not know about the workaround, since it's not really something self-explanatory.

@GaOrtiga
this can be fixed by a PR with changes in the error message.

@weizhouapache
Copy link
Member

@GaOrtiga @DaanHoogland sorry this PR is not needed in my opinion

Having a workaround should not be a reason to restrict an operation. If the operator does not know that you can circumvent this restriction they may feel obliged to perform the operation directly in the DB, especially since the exception thrown is very vague and editing the template's type to make it deletable is very non-intuitive.

If the solution is not clear, make it clear then. for example

diff --git a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
index b886f0868f6..6456ca452ed 100644
--- a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
+++ b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
@@ -749,7 +749,7 @@ public class HypervisorTemplateAdapter extends TemplateAdapterBase {
         TemplateProfile profile = super.prepareDelete(cmd);
         VMTemplateVO template = profile.getTemplate();
         if (template.getTemplateType() == TemplateType.SYSTEM) {
-            throw new InvalidParameterValueException("The DomR template cannot be deleted.");
+            throw new InvalidParameterValueException("The SYSTEM template cannot be deleted. Please update template type to USER and retry.");
         }
         checkZoneImageStores(profile.getTemplate(), profile.getZoneIdList());
         return profile;

@DaanHoogland
Copy link
Contributor

What do you think of @weizhouapache 's last sugestion @GaOrtiga ? would this satisfy both your requirements?

@GaOrtiga
Copy link
Contributor Author

@GaOrtiga @DaanHoogland sorry this PR is not needed in my opinion

Having a workaround should not be a reason to restrict an operation. If the operator does not know that you can circumvent this restriction they may feel obliged to perform the operation directly in the DB, especially since the exception thrown is very vague and editing the template's type to make it deletable is very non-intuitive.

If the solution is not clear, make it clear then. for example

diff --git a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
index b886f0868f6..6456ca452ed 100644
--- a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
+++ b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
@@ -749,7 +749,7 @@ public class HypervisorTemplateAdapter extends TemplateAdapterBase {
         TemplateProfile profile = super.prepareDelete(cmd);
         VMTemplateVO template = profile.getTemplate();
         if (template.getTemplateType() == TemplateType.SYSTEM) {
-            throw new InvalidParameterValueException("The DomR template cannot be deleted.");
+            throw new InvalidParameterValueException("The SYSTEM template cannot be deleted. Please update template type to USER and retry.");
         }
         checkZoneImageStores(profile.getTemplate(), profile.getZoneIdList());
         return profile;

@weizhouapache
While the proposed implementation provides clarity, it's not intuitive. Since changes are already necessary, instead of merely guiding users through an arbitrary process, the aim should be to enhance overall code understanding and user experience.

The force flag should be more than enough to address any remaining concerns. This flag serves as a clear indicator that the operation requires careful execution and is not intended for routine use.

Besides, deleting the current SYSTEM template does not pose a significant risk to the environment, as it can be reuploaded.

@weizhouapache
Copy link
Member

@GaOrtiga @DaanHoogland sorry this PR is not needed in my opinion

Having a workaround should not be a reason to restrict an operation. If the operator does not know that you can circumvent this restriction they may feel obliged to perform the operation directly in the DB, especially since the exception thrown is very vague and editing the template's type to make it deletable is very non-intuitive.

If the solution is not clear, make it clear then. for example

diff --git a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
index b886f0868f6..6456ca452ed 100644
--- a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
+++ b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
@@ -749,7 +749,7 @@ public class HypervisorTemplateAdapter extends TemplateAdapterBase {
         TemplateProfile profile = super.prepareDelete(cmd);
         VMTemplateVO template = profile.getTemplate();
         if (template.getTemplateType() == TemplateType.SYSTEM) {
-            throw new InvalidParameterValueException("The DomR template cannot be deleted.");
+            throw new InvalidParameterValueException("The SYSTEM template cannot be deleted. Please update template type to USER and retry.");
         }
         checkZoneImageStores(profile.getTemplate(), profile.getZoneIdList());
         return profile;

@weizhouapache
While the proposed implementation provides clarity, it's not intuitive. Since changes are already necessary, instead of merely guiding users through an arbitrary process, the aim should be to enhance overall code understanding and user experience.

Can you tell me how frequent you deleted the SYSTEM template?

The force flag should be more than enough to address any remaining concerns. This flag serves as a clear indicator that the operation requires careful execution and is not intended for routine use.

force flag is being used by other purpose.

Besides, deleting the current SYSTEM template does not pose a significant risk to the environment, as it can be reuploaded.

That is not true. Without SYSTEM template, users cannot create new networks and recreate VRs/system vms. Do you think if it is a critical issue if it happens on your production?

@GaOrtiga
Copy link
Contributor Author

@weizhouapache

Can you tell me how frequent you deleted the SYSTEM template?

Even if it's not something that is done frequently, it being purposely less intuitive impacts user experience.

That is not true. Without SYSTEM template, users cannot create new networks and recreate VRs/system vms. Do you think if it is a critical issue if it happens on your production?

While it is true that the environment will not function properly without the SYSTEM template, it can be easily reuploaded as soon as the operator realizes their mistake.

force flag is being used by other purpose.

I could add a new parameter, maybe system to make it very clear on what's the intent of the flag. This would make it so that these templates can be deleted without the need for workarounds, while adding extra security. This would also make it so that unused templates can be deleted only using the system flag, with the exception of the current template being used by the systemvms, which would require the force flag aswell as the system.

@weizhouapache
Copy link
Member

@weizhouapache

Can you tell me how frequent you deleted the SYSTEM template?

Even if it's not something that is done frequently, it being purposely less intuitive impacts user experience.

We need to compare the pros and cons ...
Comparing with possible unexpected removal of SYSTEM template, the 2 steps removal is a very minor issue.

That is not true. Without SYSTEM template, users cannot create new networks and recreate VRs/system vms. Do you think if it is a critical issue if it happens on your production?

While it is true that the environment will not function properly without the SYSTEM template, it can be easily reuploaded as soon as the operator realizes their mistake.

Are you operating a production ? If yes, you would not say this ...

force flag is being used by other purpose.

I could add a new parameter, maybe system to make it very clear on what's the intent of the flag. This would make it so that these templates can be deleted without the need for workarounds, while adding extra security. This would also make it so that unused templates can be deleted only using the system flag, with the exception of the current template being used by the systemvms, which would require the force flag aswell as the system.

IMHO do not make things too complicated...

@weizhouapache
Copy link
Member

weizhouapache commented Jan 25, 2024

I could add a new parameter, maybe system to make it very clear on what's the intent of the flag. This would make it so that these templates can be deleted without the need for workarounds, while adding extra security. This would also make it so that unused templates can be deleted only using the system flag, with the exception of the current template being used by the systemvms, which would require the force flag aswell as the system.

Although I think it is unnecessary to add an API parameter which reduces the steps from 2 to 1, for an operation which run once or twice a year, it looks ok to me as it does not lead to any risk.
If you want to implement it, go ahead. cc @GaOrtiga

we have spent too much time on this topic.

@weizhouapache
Copy link
Member

@GaOrtiga
will you support it on UI ?

API is tested ok

(localcloud) 🐱 > delete template id=d0b0a4db-1125-4a99-8cca-cf7934bbc19b
{
  "account": "admin",
  "accountid": "75837c93-aff0-11ee-8ad9-1e00940002db",
  "cmd": "org.apache.cloudstack.api.command.user.template.DeleteTemplateCmd",
  "completed": "2024-01-26T20:25:33+0000",
  "created": "2024-01-26T20:25:33+0000",
  "domainid": "476c9122-aff0-11ee-8ad9-1e00940002db",
  "domainpath": "ROOT",
  "jobid": "b9ba7dd4-58a4-4647-936a-44ca0278532f",
  "jobinstanceid": "d0b0a4db-1125-4a99-8cca-cf7934bbc19b",
  "jobinstancetype": "Template",
  "jobprocstatus": 0,
  "jobresult": {
    "errorcode": 431,
    "errortext": "Could not delete template as it is a SYSTEM template and isSystem is set to false."
  },
  "jobresultcode": 431,
  "jobresulttype": "object",
  "jobstatus": 2,
  "userid": "7584f1d6-aff0-11ee-8ad9-1e00940002db"
}
🙈 Error: async API failed for job b9ba7dd4-58a4-4647-936a-44ca0278532f
(localcloud) 🐱 > delete template id=d0b0a4db-1125-4a99-8cca-cf7934bbc19b issystem=true
{
  "success": true
}

@GaOrtiga
Copy link
Contributor Author

@GaOrtiga
will you support it on UI ?

@weizhouapache No I will not. In my opinion, adding it to the UI would risk some of the security concerns that you pointed out earlier in the discussion.

Also, I can not think of a use case that would require it as of now, and if one eventually comes up, we can implement it to the UI when needed.

@weizhouapache
Copy link
Member

@GaOrtiga
will you support it on UI ?

@weizhouapache No I will not. In my opinion, adding it to the UI would risk some of the security concerns that you pointed out earlier in the discussion.

Also, I can not think of a use case that would require it as of now, and if one eventually comes up, we can implement it to the UI when needed.

ok @GaOrtiga
so admins have to use api or cmk, right ? this does not help admins a lot.

@GaOrtiga
Copy link
Contributor Author

ok @GaOrtiga so admins have to use api or cmk, right ?

Yes.

this does not help admins a lot.

Since some security concerns have been brought up, I felt that limiting it to the API would be a good compromise in order to implement this functionality while avoiding further security discussions, since, as you mentioned, we have spent too much time on this. However, I would be happy to implement this in the UI if the community sees it as beneficial.

@weizhouapache @DaanHoogland What are your opinions on this?

@weizhouapache
Copy link
Member

@blueorangutan package

@DaanHoogland DaanHoogland added this to the 4.20.0.0 milestone Jan 30, 2024
Copy link
Member

@yadvr yadvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Would this also deletion of systemvmtemplate current in use?
  • If that's the case, can we add a check so it won't allow deleting systemvmtemplate in use
  • Does it need some UI changes, so admins can delete systemvmtemplates (not in use) from the UI?

@GaOrtiga
Copy link
Contributor Author

@rohityadavcloud

  • Would this also deletion of systemvmtemplate current in use?
  • If that's the case, can we add a check so it won't allow deleting systemvmtemplate in use

It will not allow the deletion of the current template unless both the isSystem and the forced flag are passed.

  • Does it need some UI changes, so admins can delete systemvmtemplates (not in use) from the UI?

Given some of the previous discussions regarding this implementation, mainly about security, I believe this PR should focus solely on the backend. If, in the future, we decide that UI changes are beneficial, we can make a new PR to implement it.

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8686

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-9263)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 53810 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8556-t9263-kvm-centos7.zip
Smoke tests completed. 126 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_02_trigger_shutdown Failure 341.61 test_safe_shutdown.py
test_03_secured_to_nonsecured_vm_migration Error 276.26 test_vm_life_cycle.py
test_05_rvpc_multi_tiers Failure 500.57 test_vpc_redundant.py
test_05_rvpc_multi_tiers Error 500.59 test_vpc_redundant.py

@JoaoJandre
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9279

@JoaoJandre
Copy link
Contributor

@DaanHoogland @sureshanaparti @rohityadavcloud @shwstppr could we run the CI here?

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-9858)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 53146 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8556-t9858-kvm-centos7.zip
Smoke tests completed. 126 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_events_resource Error 306.50 test_events_resource.py
test_01_events_resource Error 306.51 test_events_resource.py
test_04_deploy_vm_for_other_user_and_test_vm_operations Failure 88.70 test_network_permissions.py
ContextSuite context=TestNetworkPermissions>:teardown Error 1.43 test_network_permissions.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL Failure 389.98 test_vpc_redundant.py
test_02_redundant_VPC_default_routes Failure 979.53 test_vpc_redundant.py
test_05_rvpc_multi_tiers Failure 468.55 test_vpc_redundant.py
test_05_rvpc_multi_tiers Error 468.58 test_vpc_redundant.py

@JoaoJandre
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10461

@JoaoJandre
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10638

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10652

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-11074)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 50926 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8556-t11074-kvm-ol8.zip
Smoke tests completed. 139 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

Copy link
Contributor

@JoaoJandre JoaoJandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLGTM

@JoaoJandre
Copy link
Contributor

Merging based on approvals, CI result and manual tests (#8556 (comment))

@JoaoJandre JoaoJandre merged commit 8ca1843 into apache:main Aug 15, 2024
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Aug 22, 2024
* allow delete system VM templates

* Add isSystem parameter

* add authorized

---------

Co-authored-by: Gabriel <gabriel.fernandes@scclouds.com.br>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants